refactor merkle tree inclusion proof verification #579
Conversation
🦋 Changeset detectedLatest commit: 434f533 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| ); | ||
|
|
||
| // Calculated hash should match the root hash in the inclusion proof | ||
| return calculatedHash.equals(inclusionProof.rootHash); |
There was a problem hiding this comment.
Poking around with copilot.. it's suggesting we rewrite this as a constant time comparison function to prevent timing attacks :)
googling around, looks like there's a function in node to do this but throws on mismatch so we could wrap it like:
const { timingSafeEqual } = require('crypto');
const compare = (a, b) => {
try {
return timingSafeEqual(a, b);
} catch {
return false;
}
};
There was a problem hiding this comment.
This is a nice find! I suspect that this is being used internally as part of the crypto signature verification logic, but I'm wondering if there are other places where I should employ this 🤔
There was a problem hiding this comment.
being used internally as part of the crypto signature verification logic
Yeah would hope so!
Maybe worth a quick audit to see if we do any manual comparisons?
Signed-off-by: Brian DeHamer <bdehamer@github.com>
bdehamer
force-pushed
the
bdehamer/merkle-inclusion
branch
from
b23451c to
710ac53
Compare
Summary
Clean-up the merkle tree inclusion proof verification logic (see sigstore/protobuf-specs#82) . This was originally implemented before the Sigstore bundle format was even defined and was never integrated into the verification workflow. The change here refactors the merkle verification logic into a form that can be applied to the
TransparenclyLogEntrys found in the bundle.This logic is still NOT integrated into the verification workflow (which is why I'm not adding a changeset entry with this PR) but does prepare us for adding this in the future.